OSAC-240: Add describe networkclass CLI command and update #701
OSAC-240: Add describe networkclass CLI command and update #701SiddarthR56 wants to merge 1 commit into
Conversation
WalkthroughAdds a ChangesDescribe NetworkClass Command
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@SiddarthR56: This pull request references OSAC-240 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: SiddarthR56 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@SiddarthR56: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…with description/capabilities columns Assisted-by: Cursor/Claude
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.go`:
- Around line 87-89: The RenderNetworkClass function currently ignores errors
from fmt.Fprintf and Flush operations, violating Go error handling guidelines.
Modify RenderNetworkClass to return an error type, check and propagate errors
from all write operations (fmt.Fprintf and Flush calls within the function
body), and then at the call site where RenderNetworkClass is invoked, capture
and return the error instead of returning nil unconditionally.
- Around line 73-80: The client.List call in the anonymous function passed to
lookup.Find does not have a timeout constraint on the context, which can cause
the command to hang indefinitely if the backend stalls. Before passing ctx to
client.List, wrap it with context.WithTimeout to establish a bounded deadline
for the RPC call, ensuring the operation fails gracefully if it exceeds the
timeout duration rather than hanging indefinitely.
In `@internal/rendering/tables/osac.public.v1.NetworkClass.yaml`:
- Around line 29-35: The capability columns for IPV4 (line 29), IPV6 (line 32),
and DUAL STACK (line 35) use two-way ternary expressions that conflate missing
capabilities with false values, rendering both as 'no'. Update each of these
three value expressions to be three-way conditionals: return 'yes' when the
capability exists and is true, return '-' when capabilities are missing or the
specific capability field doesn't exist, and return 'no' when the capability
explicitly exists but is false. This aligns with how other optional fields are
handled in this table (as referenced in line 38).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 868f8f63-0c1b-4f86-9d51-c97b80121f9e
📒 Files selected for processing (6)
internal/cmd/cli/describe/describe_cmd.gointernal/cmd/cli/describe/describe_cmd_test.gointernal/cmd/cli/describe/networkclass/describe_networkclass_cmd.gointernal/cmd/cli/describe/networkclass/describe_networkclass_suite_test.gointernal/cmd/cli/describe/networkclass/describe_networkclass_test.gointernal/rendering/tables/osac.public.v1.NetworkClass.yaml
| matched, err := lookup.Find(ref, "network class", func(filter string, limit int32) ([]*publicv1.NetworkClass, error) { | ||
| resp, err := client.List(ctx, publicv1.NetworkClassesListRequest_builder{ | ||
| Filter: proto.String(filter), | ||
| Limit: proto.Int32(limit), | ||
| }.Build()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to describe network class: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "describe_networkclass_cmd.go" -type f | head -20Repository: osac-project/fulfillment-service
Length of output: 146
🏁 Script executed:
wc -l internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.goRepository: osac-project/fulfillment-service
Length of output: 148
🏁 Script executed:
cat -n internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.goRepository: osac-project/fulfillment-service
Length of output: 5855
Add an RPC deadline for the List call.
Line 74 passes ctx directly to client.List() without a timeout. If the backend stalls, this command can hang indefinitely. Wrap the List call with a bounded timeout context.
Proposed fix
import (
+ "context"
"fmt"
"io"
"log/slog"
"strings"
"text/tabwriter"
+ "time"
@@
matched, err := lookup.Find(ref, "network class", func(filter string, limit int32) ([]*publicv1.NetworkClass, error) {
- resp, err := client.List(ctx, publicv1.NetworkClassesListRequest_builder{
+ listCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ resp, err := client.List(listCtx, publicv1.NetworkClassesListRequest_builder{
Filter: proto.String(filter),
Limit: proto.Int32(limit),
}.Build())Per Go guidelines, all gRPC calls must use context.Context with cancellation and timeouts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.go` around
lines 73 - 80, The client.List call in the anonymous function passed to
lookup.Find does not have a timeout constraint on the context, which can cause
the command to hang indefinitely if the backend stalls. Before passing ctx to
client.List, wrap it with context.WithTimeout to establish a bounded deadline
for the RPC call, ensuring the operation fails gracefully if it exceeds the
timeout duration rather than hanging indefinitely.
Source: Coding guidelines
| RenderNetworkClass(c.console, matched) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the target file
fd "describe_networkclass_cmd.go" -type fRepository: osac-project/fulfillment-service
Length of output: 247
🏁 Script executed:
# Read the file to verify the code and error handling
cat -n internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.goRepository: osac-project/fulfillment-service
Length of output: 5855
🏁 Script executed:
# Search for other render functions that might have the same pattern
rg "func Render" --type go -A 5 internal/cmd/cli/describe/Repository: osac-project/fulfillment-service
Length of output: 3657
🏁 Script executed:
# Check other render functions to see if they also ignore errors
rg "func Render" -A 50 internal/cmd/cli/describe/ | grep -E "(func Render|fmt.Fprintf|Flush)" | head -40Repository: osac-project/fulfillment-service
Length of output: 4598
🏁 Script executed:
# Check one of the other render functions more closely to confirm pattern
sed -n '100,160p' internal/cmd/cli/describe/virtualnetwork/describe_virtualnetwork_cmd.go | head -70Repository: osac-project/fulfillment-service
Length of output: 1274
🏁 Script executed:
# Check how the other render functions are called to see if they also ignore errors
rg "Render(VirtualNetwork|Subnet|SecurityGroup|PublicIP|NetworkClass)" internal/cmd/cli/describe/ -B 2 -A 2Repository: osac-project/fulfillment-service
Length of output: 8615
Make RenderNetworkClass return an error and propagate it to the caller.
Lines 141–151 ignore all fmt.Fprintf and Flush errors, silently losing output failures (e.g., broken pipe). Change RenderNetworkClass to return an error, check each write operation, and propagate errors at line 87.
Proposed fix
-func RenderNetworkClass(w io.Writer, nc *publicv1.NetworkClass) {
+func RenderNetworkClass(w io.Writer, nc *publicv1.NetworkClass) error {
writer := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0)
@@
- fmt.Fprintf(writer, "ID:\t%s\n", nc.GetId())
- fmt.Fprintf(writer, "Name:\t%s\n", name)
- fmt.Fprintf(writer, "Title:\t%s\n", title)
- fmt.Fprintf(writer, "Description:\t%s\n", description)
- fmt.Fprintf(writer, "Supports IPv4:\t%s\n", supportsIPv4)
- fmt.Fprintf(writer, "Supports IPv6:\t%s\n", supportsIPv6)
- fmt.Fprintf(writer, "Supports Dual Stack:\t%s\n", supportsDualStack)
- fmt.Fprintf(writer, "Default:\t%s\n", isDefault)
- fmt.Fprintf(writer, "State:\t%s\n", state)
- fmt.Fprintf(writer, "Message:\t%s\n", message)
- writer.Flush()
+ if _, err := fmt.Fprintf(writer, "ID:\t%s\n", nc.GetId()); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Name:\t%s\n", name); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Title:\t%s\n", title); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Description:\t%s\n", description); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Supports IPv4:\t%s\n", supportsIPv4); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Supports IPv6:\t%s\n", supportsIPv6); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Supports Dual Stack:\t%s\n", supportsDualStack); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Default:\t%s\n", isDefault); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "State:\t%s\n", state); err != nil { return err }
+ if _, err := fmt.Fprintf(writer, "Message:\t%s\n", message); err != nil { return err }
+ if err := writer.Flush(); err != nil { return err }
+ return nil
}
@@
- RenderNetworkClass(c.console, matched)
+ if err := RenderNetworkClass(c.console, matched); err != nil {
+ return fmt.Errorf("failed to render network class: %w", err)
+ }Violates the Go guideline: never ignore error returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cmd/cli/describe/networkclass/describe_networkclass_cmd.go` around
lines 87 - 89, The RenderNetworkClass function currently ignores errors from
fmt.Fprintf and Flush operations, violating Go error handling guidelines. Modify
RenderNetworkClass to return an error type, check and propagate errors from all
write operations (fmt.Fprintf and Flush calls within the function body), and
then at the call site where RenderNetworkClass is invoked, capture and return
the error instead of returning nil unconditionally.
Source: Coding guidelines
| value: "has(this.capabilities) && this.capabilities.supports_ipv4? 'yes': 'no'" | ||
|
|
||
| - header: IPV6 | ||
| value: "has(this.capabilities) && this.capabilities.supports_ipv6? 'yes': 'no'" | ||
|
|
||
| - header: DUAL STACK | ||
| value: "has(this.capabilities) && this.capabilities.supports_dual_stack? 'yes': 'no'" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the NetworkClass YAML file
find . -name "*NetworkClass*.yaml" -type fRepository: osac-project/fulfillment-service
Length of output: 199
🏁 Script executed:
# Check the actual content of the file at the specified lines
cat -n internal/rendering/tables/osac.public.v1.NetworkClass.yaml | sed -n '20,40p'Repository: osac-project/fulfillment-service
Length of output: 774
🏁 Script executed:
# Search for the renderer mentioned in the scratchpad
find . -path "*/internal/cmd/cli/describe/networkclass/*describe_networkclass_cmd.go" -type fRepository: osac-project/fulfillment-service
Length of output: 146
🏁 Script executed:
# Check if there's any CEL-related logic or tests that validate these expressions
rg "supports_ipv4|supports_ipv6|supports_dual_stack" -A 2 -B 2Repository: osac-project/fulfillment-service
Length of output: 12260
Fix capability columns to distinguish missing vs. false values.
Lines 29, 32, 35 currently conflate absent capabilities with explicit false, rendering both as 'no'. Use a three-way expression to return '-' when capabilities are missing, consistent with other optional fields in this table (see line 38).
Proposed fix
- header: IPV4
- value: "has(this.capabilities) && this.capabilities.supports_ipv4? 'yes': 'no'"
+ value: "has(this.capabilities)? (this.capabilities.supports_ipv4? 'yes': 'no'): '-'"
- header: IPV6
- value: "has(this.capabilities) && this.capabilities.supports_ipv6? 'yes': 'no'"
+ value: "has(this.capabilities)? (this.capabilities.supports_ipv6? 'yes': 'no'): '-'"
- header: DUAL STACK
- value: "has(this.capabilities) && this.capabilities.supports_dual_stack? 'yes': 'no'"
+ value: "has(this.capabilities)? (this.capabilities.supports_dual_stack? 'yes': 'no'): '-'"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/rendering/tables/osac.public.v1.NetworkClass.yaml` around lines 29 -
35, The capability columns for IPV4 (line 29), IPV6 (line 32), and DUAL STACK
(line 35) use two-way ternary expressions that conflate missing capabilities
with false values, rendering both as 'no'. Update each of these three value
expressions to be three-way conditionals: return 'yes' when the capability
exists and is true, return '-' when capabilities are missing or the specific
capability field doesn't exist, and return 'no' when the capability explicitly
exists but is false. This aligns with how other optional fields are handled in
this table (as referenced in line 38).
3 participants
Summary
Summary by CodeRabbit